Add default arguments ability and update tests. #91
Conversation
|
|
|
Having thought about it, I don't think that it is sensible to have a default 'handle'. It makes sense to want to work in engineering or physics units, or to work with the sim or live, but not to work with 'setpoint' all the time. I suggest we remove that aspect from the pull request. |
pytac/__init__.py
Outdated
| ENG = 'engineering' | ||
| PHYS = 'physics' | ||
| # Data Source types. | ||
| SIM = 'simulation' | ||
| LIVE = 'live' | ||
| # Default argument flag. | ||
| default = 'default' |
There was a problem hiding this comment.
This is a constant so should be in all caps i.e.
DEFAULT = 'default'
|
Ok I will remove default handle and put it back to how it was with get_value using pytac.RB as the default specified argument and set_value using pytac.SP. |
pytac/data_source.py
Outdated
| units = self._default_units | ||
| if data_source is pytac.default: | ||
| if data_source is pytac.DEFAULT: |
There was a problem hiding this comment.
You should really use == not is in this case. I think it's unlikely to cause you a problem, but there's a chance that you end up with a string default that won't compare correctly as DEFAULT.
There was a problem hiding this comment.
That stack overflow makes sense, interestingly I currently can't cause that problem in our code, but I will change it just in case.
…t for element creation and update tests.
pytac/lattice.py
Outdated
| element.set_value(field, value) | ||
| element.set_value(field, value, handle=pytac.SP) | ||
|
|
||
| def set_default_arguments(self, default_units=None, |
There was a problem hiding this comment.
It now looks a bit strange for this to be one method - I suggest you split it into two.
pytac/lattice.py
Outdated
| raise LatticeException('Please set at least one default argument ' | ||
| 'for units or data_source.') | ||
| if default_units == pytac.ENG or default_units == pytac.PHYS: | ||
| self._data_source_manager._default_units = default_units |
There was a problem hiding this comment.
You're using a private attribute _default_units from outside the DataSourceManager class. Should it be a public attribute?
pytac/lattice.py
Outdated
| return self._data_source_manager.default_units | ||
|
|
||
| def get_default_data_source(self): | ||
| """Get the default data source, pytac.RB or pytac.SP. |
pytac/lattice.py
Outdated
| pytac.LIVE, pytac.SIM)) | ||
|
|
||
| def get_default_units(self): | ||
| """Get the default unit type, pytac.RB or pytac.SP. |
|
I'm finding it quite hard to think through all the consequences of these changes. I've made a couple of comments about the docstrings, then I'll merge and we'll see how we get on. |
|
Fixed those two mistakes. I'm not sure if there are many consequences, as pytac will function as it does currently if the defaults are not changed and I don't think there are any complications when defaults are set as the use cases are pretty limited. |
Changes made - ready to merge again.
I also added tests for the template classes Device and DataSource, as ControlSystem already had them.
So I have renamed test_cs to test_invalid_classes, all that it does is checks all methods on all the template classes raise NotImplementedError.